Fix #80, support an include mechanism for PDI config by JAuriac · Pull Request #653 · pdidev/pdi

JAuriac · 2026-03-18T15:17:09Z

Support an include mechanism for PDI config, to include other YAML files into root YAML document.
Depends on Paraconf handling of relative path to point to included YAML files.

List of things to check before making a PR

Before merging your code, please check the following:

jbigot · 2026-03-19T10:09:51Z

+	PC_tree_t pdi = PC_get(conf, ".pdi");
+	if (!PC_status(pdi)) {
+		root = pdi;
+	}


please don't do that ! It's up to the user to do that if they want.

Maybe the syntax could support 2 variants:
either:

include: - /path/to/my/file.yaml

or

include: - file: /path/to/my/file.yaml node: .pdi[5]

where node is any valid ypath (paraconf) expression

JAuriac · 2026-03-19T14:04:34Z

As we currently need the full path to read a yaml subfile, the test_07 works on the github runner for linux (through a workaround overwriting a placeholder field in the root yaml file) but not for macos, and not for local

JAuriac · 2026-03-28T15:48:24Z

The integrations tests "test_duplicate_yaml_include" and "test_circular_yaml_include" are disabled by default, as they expect failure. We use instead the unitary tests "check_duplicate" and "load_pdi_config_duplicate_data_via_inclusion", using System_error.
This last commit only test using absolute paths for the CI, with Paraconf 1.0.3 (also tested locally for both absolute and relative paths, with Paraconf 1.1.1).

Yushan-Wang · 2026-03-31T12:53:39Z

+#ifdef PARACONF_VERSION_MAJOR
+#if (PARACONF_VERSION_MAJOR > 1) || (PARACONF_VERSION_MAJOR == 1 && PARACONF_VERSION_MINOR >= 1)
+#define PDI_HAS_PC_PATH 1
+#else
+#define PDI_HAS_PC_PATH 0
+#endif
+#else
+#define PDI_HAS_PC_PATH 0
+#endif
+


is this really necessary?

Yes, to use absolute paths with Paraconf < 1.1, and both absolute and relative paths with Paraconf 1.1+

I'd recommend this pattern:

Suggested change

#ifdef PARACONF_VERSION_MAJOR

#if (PARACONF_VERSION_MAJOR > 1) || (PARACONF_VERSION_MAJOR == 1 && PARACONF_VERSION_MINOR >= 1)

#define PDI_HAS_PC_PATH 1

#else

#define PDI_HAS_PC_PATH 0

#endif

#else

#define PDI_HAS_PC_PATH 0

#endif

#if defined(PARACONF_VERSION) && (PARACONF_UNTYPED_VERSION >= PARACONF_UNTYPED_COMPUTE_VERSION(1, 1, 0))

#define PDI_HAS_PC_PATH 1

#else

#define PDI_HAS_PC_PATH 0

#endif

of you could directly do the test where needed

Yushan-Wang · 2026-03-31T12:55:07Z

 namespace {

-void load_data(Context& ctx, PC_tree_t node, bool is_metadata)
+void load_data(Global_context& ctx, PC_tree_t node, bool is_metadata)


why changing to global_context?

Changed back

Changed again, to use make_and_check_descriptor() which is accessible only from Global_context (and not Context)

Yushan-Wang · 2026-03-31T12:58:57Z

+		std::unordered_set<std::string> visited;
+		collect_plugins_impl(conf, visited);


why this is required?

This was used to handle the dual pass. We are now back to a single pass on the PC tree, with two sub-pass to read plugins, then types/metadata/data.

jbigot · 2026-04-02T09:02:55Z

+#ifdef PARACONF_VERSION_MAJOR
+#if (PARACONF_VERSION_MAJOR > 1) || (PARACONF_VERSION_MAJOR == 1 && PARACONF_VERSION_MINOR >= 1)
+#define PDI_HAS_PC_PATH 1
+#else
+#define PDI_HAS_PC_PATH 0
+#endif
+#else
+#define PDI_HAS_PC_PATH 0
+#endif
+


I'd recommend this pattern:

Suggested change

#ifdef PARACONF_VERSION_MAJOR

#if (PARACONF_VERSION_MAJOR > 1) || (PARACONF_VERSION_MAJOR == 1 && PARACONF_VERSION_MINOR >= 1)

#define PDI_HAS_PC_PATH 1

#else

#define PDI_HAS_PC_PATH 0

#endif

#else

#define PDI_HAS_PC_PATH 0

#endif

#if defined(PARACONF_VERSION) && (PARACONF_UNTYPED_VERSION >= PARACONF_UNTYPED_COMPUTE_VERSION(1, 1, 0))

#define PDI_HAS_PC_PATH 1

#else

#define PDI_HAS_PC_PATH 0

#endif

of you could directly do the test where needed

jbigot · 2026-04-08T10:30:02Z


 Global_context::Global_context(PC_tree_t conf)
 	: m_logger{"PDI", PC_get(conf, ".logging")}
 	, m_plugins{*this, conf}


this will load the plugins at the construction of the object and should be reworked

jbigot · 2026-04-13T14:41:24Z

+	load_pdi_config(conf); // single tree traversal + two flat sub-passes

 	// evaluate pattern after loading plugins
 	m_logger.evaluate_pattern(*this);


shouldn't this be par of load_pdi_config?

I kept this separated as it previously was, to not make a unique monolithic function inside of the Global_context definition

jbigot · 2026-04-13T14:42:04Z

+void Global_context::check_duplicate(const PC_tree_t& node, const std::string& name)
+{
+	auto [it, inserted] = m_defined.emplace(name, node);
+	if (!inserted) {
+		// it->second = first definition site (deepest file in include tree, post-order)
+		// node = redefinition site (current file being processed)
+#if PDI_HAS_PC_PATH
+		const char* orig_path = PC_path(it->second);
+		const char* redef_path = PC_path(node);
+
+		bool orig_has_path = orig_path && fs::exists(orig_path);
+		bool redef_has_path = redef_path && fs::exists(redef_path);
+
+		if (redef_has_path && orig_has_path) {
+			throw Config_error(node, "Duplicate definition of '{}', originally defined in '{}', defined again in '{}'", name, redef_path, orig_path);
+		} else if (redef_has_path) {
+			throw Config_error(node, "Duplicate definition of '{}', originally defined in string config, defined again in '{}'", name, redef_path);
+		} else if (orig_has_path) {
+			throw Config_error(node, "Duplicate definition of '{}', originally defined in '{}', defined again in a string config", name, orig_path);
+		}
+#endif
+		throw Config_error(node, "Duplicate definition of '{}'", name);
+	}
+}


Don't think you need a dedicated function

Can we get the 'Config_error' with the lines numbers without a new function ?

jbigot · 2026-04-13T14:43:47Z

+	void collect_ordered_nodes_impl(
+		PC_tree_t conf,
+		std::unordered_set<std::string>& globally_loaded,
+		std::unordered_set<std::string>& include_chain,
+		std::vector<std::pair<std::string, PC_tree_t>>& ordered_nodes,
+		std::size_t& no_path_counter,
+		const std::string& known_path = "",
+		PC_tree_t include_directive = {},
+		const std::string& parent_id = ""
+	);


cound't this be a static function in the cxx?

'collect_ordered_nodes_impl', yes, but not 'collect_ordered_nodes', as we need a public member, used by 'load_pdi_config' ?

Moved both collect_ordered_nodes() and collect_ordered_nodes() to the local namespace instead

jbigot · 2026-04-13T14:43:58Z

+	/// Traverses the include tree once in post-order (deepest includes first),
+	/// filling `ordered_nodes` with (canonical_id, PC_tree_t) pairs.
+	/// Diamond/circular detection is handled here.
+	void collect_ordered_nodes(
+		PC_tree_t conf,
+		std::unordered_set<std::string>& globally_loaded,
+		std::unordered_set<std::string>& include_chain,
+		std::vector<std::pair<std::string, PC_tree_t>>& ordered_nodes,
+		const std::string& known_path = "",
+		PC_tree_t include_directive = {},
+		const std::string& parent_id = ""
+	);


cound't this be a static function in the cxx?

'collect_ordered_nodes_impl', yes, but not 'collect_ordered_nodes', as we need a public member, used by 'load_pdi_config' ?

Moved both collect_ordered_nodes() and collect_ordered_nodes() to the local namespace instead

Co-authored-by: Julian Auriac <julian.auriac@cea.fr>

JAuriac requested review from Yushan-Wang, benoitmartin88 and jbigot as code owners March 18, 2026 15:17

JAuriac linked an issue Mar 18, 2026 that may be closed by this pull request

Support an include mechanism for PDI config #80

Open

Yushan-Wang reviewed Mar 18, 2026

View reviewed changes

Comment thread pdi/src/global_context.cxx Outdated

Yushan-Wang reviewed Mar 18, 2026

View reviewed changes

Comment thread tests/test_07.yml Outdated

Yushan-Wang reviewed Mar 18, 2026

View reviewed changes

Comment thread tests/test_07_data.yml Outdated

Yushan-Wang reviewed Mar 18, 2026

View reviewed changes

Comment thread tests/test_07.yml Outdated

Yushan-Wang reviewed Mar 19, 2026

View reviewed changes

Comment thread pdi/src/global_context.cxx Outdated

Yushan-Wang reviewed Mar 19, 2026

View reviewed changes

Comment thread tests/test_07_data.yml Outdated

jbigot reviewed Mar 19, 2026

View reviewed changes

Yushan-Wang assigned JAuriac Mar 25, 2026